Refactor findNodeHandler#6736

Merged

piaskowyk merged 13 commits intomainfrom

@piaskowyk/findnodehandler-refactor

Dec 4, 2024

Member

piaskowyk commented Nov 20, 2024 •

edited

Loading

Summary

This PR addresses the issue reported at #6719 and aims to:

Unify the method used to obtain the view tag, as there are currently several approaches.
Avoid passing a class component to the findNodeHandler. Instead, we'll pass a ref to the component, similar to what Expo implemented here: [core] Use findNodeHandle with a native ref instead of class component instance expo/expo#33016.
Limit unnecessary invocations of findNodeHandler to no more than one call per render.
Remove the invocation of findHostInstance from Paper renderer on the New Architecture.

Additional Remarks:

When a class component is passed to createAnimatedComponent, it will still fall back to the slow path where we can get a native ref.
In NativeEventManager, we need to call findNodeHandler again after every render to ensure that the children of <GestureDetector /> haven't changed their native tags.
LayoutAnimationConfig still uses findNodeHandler. It requires a complete refactor of their functionality to eliminate its use, and I plan to handle this in another PR.
findHostInstance_DEPRECATED always follows the slow path even for native refs, which is why I've implemented our own version of findHostInstance to optimize the happy path.

Fixes #6719

Related PRs:

Test plan

check Paper
check Fabric
check Web


          Refactor findNodeHandler

93578ee

piaskowyk mentioned this pull request

Renimated calls findNodeHandle with class instances, which is a slow path in React #6719

Closed


          Update Fabric

a4204f0

tjzel approved these changes

View reviewed changes

Collaborator

tjzel left a comment

Looks good to me overall. Make sure to test it with the newest Gesture Handler and SVG on their more complex examples, since they used to have cases where these refs mechanisms broke.

packages/react-native-reanimated/src/createAnimatedComponent/createAnimatedComponent.tsx Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/createAnimatedComponent/createAnimatedComponent.tsx Show resolved Hide resolved

packages/react-native-reanimated/src/createAnimatedComponent/createAnimatedComponent.tsx Outdated Show resolved Hide resolved

piaskowyk added 4 commits

November 22, 2024 09:37


          Update events

37b6c87


          Linter

835aca5


          Improvements after review

43305eb


          Conditions reorder

f699fb7

piaskowyk marked this pull request as ready for review

November 22, 2024 09:07

piaskowyk requested review from tjzel and tomekzaw

November 22, 2024 09:07

tjzel requested changes

View reviewed changes

packages/react-native-reanimated/src/createAnimatedComponent/NativeEventsManager.ts Show resolved Hide resolved

packages/react-native-reanimated/src/createAnimatedComponent/NativeEventsManager.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/createAnimatedComponent/NativeEventsManager.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/createAnimatedComponent/NativeEventsManager.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/createAnimatedComponent/NativeEventsManager.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/createAnimatedComponent/createAnimatedComponent.tsx Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/createAnimatedComponent/commonTypes.ts Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

piaskowyk added 3 commits

November 22, 2024 13:20


          Improvements after review

fdeee9f


          Improve Fabric-Paper compatibility

44fe723


          Remove console log

c169616

piaskowyk requested a review from tjzel

November 22, 2024 12:42

piaskowyk mentioned this pull request

Replace findNodeHandle with reading _componentViewTag #6720

Merged


          Merge

tomekzaw reviewed

View reviewed changes

packages/react-native-reanimated/src/createAnimatedComponent/NativeEventsManager.ts Show resolved Hide resolved

packages/react-native-reanimated/src/createAnimatedComponent/createAnimatedComponent.tsx Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/fabricUtils.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

tjzel reviewed

View reviewed changes

packages/react-native-reanimated/src/createAnimatedComponent/NativeEventsManager.ts Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved


          Improvements

6a0c4c6

piaskowyk requested review from tjzel and tomekzaw

November 25, 2024 10:30

tomekzaw mentioned this pull request

Bump example apps to React Native 0.77 #6768

Merged


          Add missing comment

7a41e4e

tjzel approved these changes

View reviewed changes

Collaborator

tjzel left a comment

Let's go!

packages/react-native-reanimated/src/platform-specific/findHostInstance.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/findHostInstance.ts Outdated Show resolved Hide resolved

packages/react-native-reanimated/src/platform-specific/RNRenderer.ts Outdated Show resolved Hide resolved


          Types improvements

b4e1063

tomekzaw approved these changes

View reviewed changes

piaskowyk enabled auto-merge

December 4, 2024 10:08

piaskowyk added this pull request to the merge queue

Merged via the queue into main with commit 5ffa477

piaskowyk deleted the @piaskowyk/findnodehandler-refactor branch

December 4, 2024 10:09

tsapeta mentioned this pull request

Expo52: Can not animate BlurView intensity with reanimated expo/expo#32781

Closed

rjantz2 mentioned this pull request

Animated.View produces errors when StrictMode is enabled #6754

Closed

piaskowyk added a commit that referenced this pull request


          cherry-pick: Refactor findNodeHandler (#6736)

64cbdf7

piaskowyk added a commit that referenced this pull request


          cherry-pick: Refactor findNodeHandler (#6736)

80ba01f

piaskowyk mentioned this pull request

Release 3.16.5 #6804

Merged

piaskowyk added a commit that referenced this pull request


          cherry-pick: Refactor findNodeHandler (#6736)

574ebb5

tjzel pushed a commit that referenced this pull request


          Refactor findNodeHandler (#6736)

ca4f9f0

## Summary

This PR addresses the issue reported at
[https://github.com/software-mansion/react-native-reanimated/issues/6719](https://github.com/software-mansion/react-native-reanimated/issues/6719)
and aims to:

- **Unify the method used to obtain the view tag**, as there are
currently several approaches.
- **Avoid passing a class component to the findNodeHandler**. Instead,
we'll pass a ref to the component, similar to what Expo implemented
here:
[https://github.com/expo/expo/pull/33016](https://github.com/expo/expo/pull/33016).
- **Limit unnecessary invocations of findNodeHandler** to no more than
one call per render.
- **Remove the invocation of findHostInstance from Paper renderer** on
the New Architecture.

**Additional Remarks:**
- When a class component is passed to `createAnimatedComponent`, it will
still fall back to the slow path where we can get a native ref.
- In `NativeEventManager`, we need to call findNodeHandler again after
every render to ensure that the children of `<GestureDetector />`
haven't changed their native tags.
- `LayoutAnimationConfig` still uses findNodeHandler. It requires a
complete refactor of their functionality to eliminate its use, and I
plan to handle this in another PR.
- `findHostInstance_DEPRECATED` always follows the slow path even for
native refs, which is why I've implemented our own version of
`findHostInstance` to optimize the happy path.

Fixes
#6719

Related PRs:
- #6030
- #5960
- #4445
- expo/expo#33016

## Test plan

- [x] check Paper
- [x] check Fabric
- [x] check Web

tjzel pushed a commit that referenced this pull request


          Refactor findNodeHandler (#6736)

d381387

## Summary

This PR addresses the issue reported at
[https://github.com/software-mansion/react-native-reanimated/issues/6719](https://github.com/software-mansion/react-native-reanimated/issues/6719)
and aims to:

- **Unify the method used to obtain the view tag**, as there are
currently several approaches.
- **Avoid passing a class component to the findNodeHandler**. Instead,
we'll pass a ref to the component, similar to what Expo implemented
here:
[https://github.com/expo/expo/pull/33016](https://github.com/expo/expo/pull/33016).
- **Limit unnecessary invocations of findNodeHandler** to no more than
one call per render.
- **Remove the invocation of findHostInstance from Paper renderer** on
the New Architecture.

**Additional Remarks:**
- When a class component is passed to `createAnimatedComponent`, it will
still fall back to the slow path where we can get a native ref.
- In `NativeEventManager`, we need to call findNodeHandler again after
every render to ensure that the children of `<GestureDetector />`
haven't changed their native tags.
- `LayoutAnimationConfig` still uses findNodeHandler. It requires a
complete refactor of their functionality to eliminate its use, and I
plan to handle this in another PR.
- `findHostInstance_DEPRECATED` always follows the slow path even for
native refs, which is why I've implemented our own version of
`findHostInstance` to optimize the happy path.

Fixes
#6719

Related PRs:
- #6030
- #5960
- #4445
- expo/expo#33016

## Test plan

- [x] check Paper
- [x] check Fabric
- [x] check Web

tjzel pushed a commit that referenced this pull request


          Refactor findNodeHandler (#6736)

e64257a

## Summary

This PR addresses the issue reported at
[https://github.com/software-mansion/react-native-reanimated/issues/6719](https://github.com/software-mansion/react-native-reanimated/issues/6719)
and aims to:

- **Unify the method used to obtain the view tag**, as there are
currently several approaches.
- **Avoid passing a class component to the findNodeHandler**. Instead,
we'll pass a ref to the component, similar to what Expo implemented
here:
[https://github.com/expo/expo/pull/33016](https://github.com/expo/expo/pull/33016).
- **Limit unnecessary invocations of findNodeHandler** to no more than
one call per render.
- **Remove the invocation of findHostInstance from Paper renderer** on
the New Architecture.

**Additional Remarks:**
- When a class component is passed to `createAnimatedComponent`, it will
still fall back to the slow path where we can get a native ref.
- In `NativeEventManager`, we need to call findNodeHandler again after
every render to ensure that the children of `<GestureDetector />`
haven't changed their native tags.
- `LayoutAnimationConfig` still uses findNodeHandler. It requires a
complete refactor of their functionality to eliminate its use, and I
plan to handle this in another PR.
- `findHostInstance_DEPRECATED` always follows the slow path even for
native refs, which is why I've implemented our own version of
`findHostInstance` to optimize the happy path.

Fixes
#6719

Related PRs:
- #6030
- #5960
- #4445
- expo/expo#33016

## Test plan

- [x] check Paper
- [x] check Fabric
- [x] check Web

kkafar mentioned this pull request

refactor: refactor findHostInstance following reanimated software-mansion/react-native-screens#2609

Closed

8 tasks

winghouchan mentioned this pull request

findHostInstance_DEPRECATED is deprecated and findNodeHandle is deprecated errors when using Animated.View inside react-native-gesture-handler's GestureDetector #6997

Open

gorhom mentioned this pull request

[Bug]: Bottom sheet does not always open when React's Strict Mode is enabled gorhom/react-native-bottom-sheet#2155

Closed

r0h0gg6 pushed a commit to r0h0gg6/react-native-reanimated that referenced this pull request


          Refactor findNodeHandler (software-mansion#6736)

dd67875

## Summary

This PR addresses the issue reported at
[https://github.com/software-mansion/react-native-reanimated/issues/6719](https://github.com/software-mansion/react-native-reanimated/issues/6719)
and aims to:

- **Unify the method used to obtain the view tag**, as there are
currently several approaches.
- **Avoid passing a class component to the findNodeHandler**. Instead,
we'll pass a ref to the component, similar to what Expo implemented
here:
[https://github.com/expo/expo/pull/33016](https://github.com/expo/expo/pull/33016).
- **Limit unnecessary invocations of findNodeHandler** to no more than
one call per render.
- **Remove the invocation of findHostInstance from Paper renderer** on
the New Architecture.

**Additional Remarks:**
- When a class component is passed to `createAnimatedComponent`, it will
still fall back to the slow path where we can get a native ref.
- In `NativeEventManager`, we need to call findNodeHandler again after
every render to ensure that the children of `<GestureDetector />`
haven't changed their native tags.
- `LayoutAnimationConfig` still uses findNodeHandler. It requires a
complete refactor of their functionality to eliminate its use, and I
plan to handle this in another PR.
- `findHostInstance_DEPRECATED` always follows the slow path even for
native refs, which is why I've implemented our own version of
`findHostInstance` to optimize the happy path.

Fixes
software-mansion#6719

Related PRs:
- software-mansion#6030
- software-mansion#5960
- software-mansion#4445
- expo/expo#33016

## Test plan

- [x] check Paper
- [x] check Fabric
- [x] check Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet